Skip to content

0.6.0 testflight feedback crashes#1371

Merged
sosnovsky merged 1 commit intomasterfrom
feature/issue-1365
Feb 12, 2022
Merged

0.6.0 testflight feedback crashes#1371
sosnovsky merged 1 commit intomasterfrom
feature/issue-1365

Conversation

@ivan-ushakov
Copy link
Contributor

close #1365


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@ivan-ushakov
Copy link
Contributor Author

@tomholub
Are you OK with having such amount of throw and try in code?

@sosnovsky
Copy link
Collaborator

I think this implementation won't fix initial issue - currently var storage: Realm is a computed property (https://github.com/FlowCrypt/flowcrypt-ios/blob/master/FlowCrypt/Functionality/DataManager/Encrypted%20Storage/EncryptedStorage.swift#L71) and it's re-initialized each time when app uses encryptedStorage.storage.
So possibly we have crashes when app tries to perform multiple Realm requests simultaneously.

My proposal here is to initialize let storage: Realm only once in the init of EncryptedStorage and then check Testflight crash logs to see if these crashes are gone or still there.

@tomholub
Copy link
Collaborator

tomholub commented Feb 9, 2022

My proposal here is to initialize let storage: Realm only once in the init of EncryptedStorage and then check Testflight crash logs to see if these crashes are gone or still there.

👍 I've additionally still been seeing the "red button screen of death" (storage corrupted) view and had to reset storage just today. This may be one of the reasons. We should only initialize it once.

@ivan-ushakov
Copy link
Contributor Author

My proposal here is to initialize let storage: Realm only once in the init of EncryptedStorage and then check Testflight crash logs to see if these crashes are gone or still there.

It is possible but since Realm attaches to thread and we have different places where we use Realm, we have use one thread. So it should be either actor or queue and async facade, like we did for Google libraries. This could affect performance.

@sosnovsky
Copy link
Collaborator

It is possible but since Realm attaches to thread and we have different places where we use Realm, we have use one thread. So it should be either actor or queue and async facade, like we did for Google libraries. This could affect performance.

Yeah, you're right - it'll be more complex than just re-initializing Realm on the same thread as current code.

But maybe crashes from #1365 are already fixed by some other changes - I checked Testflight logs from Xcode and the last recorded crash was on February 1. Then on February 2 we merged #1331 to master and no crashes with EncryptedStorage.storage happened since then.

Also these crashes were related to DataService.currentUser getter, but we removed DataService in yesterday's PR, it can help to avoid this Realm issue too.

However I noticed new crashes which appeared in 0.7.0 (attaching crashlog), I think we should find what causes them and check if they're related to Realm initialisation too.

testflight_feedback.zip

@tomholub
Copy link
Collaborator

I checked Testflight logs from Xcode and the last recorded crash was on February 1. Then on February 2 we merged #1331 to master and no crashes with EncryptedStorage.storage happened since then.

Testflight crashes are not affected by what gets pushed to master, only by what is sent to apple for review (and what users end up using). I only upload new versions to apple around the time I make a versioned commit on master.

Therefore, the lack of reports is not caused by any merged PR around that time. More likely, users that got crashes walked away and didn't try again, or the crash self-recovered on next app startup - hard to say.

@tomholub
Copy link
Collaborator

However I noticed new crashes which appeared in 0.7.0 (attaching crashlog), I think we should find what causes them and check if they're related to Realm initialisation too.

testflight_feedback.zip

Thanks for having a look

@sosnovsky
Copy link
Collaborator

Therefore, the lack of reports is not caused by any merged PR around that time. More likely, users that got crashes walked away and didn't try again, or the crash self-recovered on next app startup - hard to say.

Then I think we should merge this PR with added try/throw handling to avoid crashes on Realm initialisation.
Maybe in the future we'll find more elegant way of handling it, but for now current approach should make app more stable.

@tomholub
Copy link
Collaborator

ok

@ivan-ushakov ivan-ushakov marked this pull request as ready for review February 12, 2022 19:02
@sosnovsky sosnovsky merged commit f1a7dff into master Feb 12, 2022
@sosnovsky sosnovsky deleted the feature/issue-1365 branch February 12, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.6.0 testflight feedback crashes

3 participants